Warn about changing `query_constraints:` behavior #51571

nvasilevski · 2024-04-15T16:03:42Z

This PR adds a deprecation warning for the query_constraints: association option. This option will change behavior in the future versions of Rails and applications are encouraged to switch to foreign_key: to preserve the current behavior.

Related to #49671 (comment)

Deprecation location

Currently Rails will issue a deprecation during boot but there is an option to move it and only warn on runtime when a reflection with query_constraints is used.
I chose to put it in the initialize just because it's the simplest available solution and moving it to runtime will require a bit of renaming.
Let me know if you think there is a reason to have this deprecation issued in runtime or if we should have both.

Test models

To avoid having this deprecation in tests I migrated all models to foreign_key: but the long term intention is for Sharded:: namespace models to use the new behavior of query_constrainsts and Cpk:: to keep using foreign_key

Also passing CI is an assurance that foreign_key is indeed a safe substitution for query_constraints:

eileencodes · 2024-04-17T14:01:06Z

activerecord/lib/active_record/reflection.rb

+          ActiveRecord.deprecator.warn <<~MSG.squish
+            Setting `query_constraints:` option on `#{active_record}.#{macro} :#{name}` is deprecated
+            and its behavior may be modifed in Rails 8.
+            To preserve the current behavior please use the `foreign_key` option instead.


I'm not sure exactly why but the text here feels confusing. It's not immediately clear that foreign key is a replacement for query constraints. The implication that it "might" be different in the future isn't clear. Maybe something more like:

Setting `query_constraints:` option on `#{active_record}.#{macro} :#{name}` is deprecated. To maintain current behavior, use the `foreign_key` option instead.

Yeah so I initially came up with a straightforward message like you suggested but then I tried to convey that query_constraints: is not going away, there will just be new behavior which may actually fit your application better but at the same time we can't promise anything until we release the actual new behavior.

So I'll apply your suggestion, it's better to be straightforward as foreign_key being a direct substitution is what we can guarantee at the moment.

Thanks!

eileencodes · 2024-04-17T14:01:54Z

activerecord/test/cases/associations_test.rb

+      Setting `query_constraints:` option on `Sharded::BlogPost.has_many :qc_deprecated_comments` is deprecated
+      and its behavior may be modifed in Rails 8.
+      To preserve the current behavior please use the `foreign_key` option instead.
+    MSG


We don't enforce this style but generally prefer a new line between MSG and the assertion. It makes the test easier to read.

100% agree. That was an overlook on my side, will change

eileencodes · 2024-04-17T14:03:04Z

activerecord/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Warn about changing `query_constraints:` behavior


This should explain the change rather than the behavior.

Suggested change

* Warn about changing `query_constraints:` behavior

* In association option `query_constraints` is deprecated in favor of `foreign_key`.

eileencodes · 2024-04-17T14:03:30Z

activerecord/CHANGELOG.md

+*   Warn about changing `query_constraints:` behavior
+
+    `query_constraints:` association will change behavior in the future versions of Rails
+    and applications are advised to switch to `foreign_key:` to preserve the current behavior.


I think it's confusing to say that behavior will change but not to what. Applications just need to know to switch, not that there's some other future change.

Yeah I agree, since we can't promise anything concrete it's best to just ask applications to migrate to avoid any confusion in the future. I'll drop the "future versions" bit

eileencodes

Looks good, can you fix the conflicts and then I'll merge?

This commit adds a deprecation warning for the `query_constraints:` association option. This option will change behavior in the future versions of Rails and applications are encouraged to switch to `foreign_key:` to preserve the current behavior.

nvasilevski · 2024-05-08T20:21:50Z

Looks good, can you fix the conflicts and then I'll merge?

Done!

This pull request replaces the deprecated `query_constraints:` option with `foreign_key` for destroy_async test models. Follow up rails#51571 - This commit addresses these two warnings below: ``` % bin/test test/activejob/destroy_association_async_test.rb Using sqlite3 DEPRECATION WARNING: Setting `query_constraints:` option on `Cpk::BookDestroyAsync.has_many :chapters` is deprecated. To maintain current behavior, use the `foreign_key` option instead. (called from new at /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19) /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `new' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `create' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:50:in `create_reflection' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:32:in `build' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations.rb:1531:in `has_many' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/book_destroy_async.rb:7:in `<class:BookDestroyAsync>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/book_destroy_async.rb:4:in `<module:Cpk>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/book_destroy_async.rb:3:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/activerecord/test/activejob/destroy_association_async_test.rb:27:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:61:in `block in load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `each' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:52:in `run' /Users/yahonda/src/github.com/rails/rails/activerecord/test/support/tools.rb:37:in `<top (required)>' bin/test:11:in `require_relative' bin/test:11:in `<main>' DEPRECATION WARNING: Setting `query_constraints:` option on `Cpk::ChapterDestroyAsync.belongs_to :book` is deprecated. To maintain current behavior, use the `foreign_key` option instead. (called from new at /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19) /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `new' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `create' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:50:in `create_reflection' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:32:in `build' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations.rb:1910:in `belongs_to' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/chapter_destroy_async.rb:8:in `<class:ChapterDestroyAsync>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/chapter_destroy_async.rb:4:in `<module:Cpk>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/chapter_destroy_async.rb:3:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/activerecord/test/activejob/destroy_association_async_test.rb:28:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:61:in `block in load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `each' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:52:in `run' /Users/yahonda/src/github.com/rails/rails/activerecord/test/support/tools.rb:37:in `<top (required)>' bin/test:11:in `require_relative' bin/test:11:in `<main>' Run options: --seed 24461 .................... Finished in 0.199597s, 100.2019 runs/s, 370.7471 assertions/s. 20 runs, 74 assertions, 0 failures, 0 errors, 0 skips % ```

This pull request replaces the deprecated `query_constraints:` option with `foreign_key` for destroy_async test models. Follow up #51571 - This commit addresses these two warnings below: ``` % bin/test test/activejob/destroy_association_async_test.rb Using sqlite3 DEPRECATION WARNING: Setting `query_constraints:` option on `Cpk::BookDestroyAsync.has_many :chapters` is deprecated. To maintain current behavior, use the `foreign_key` option instead. (called from new at /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19) /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `new' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `create' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:50:in `create_reflection' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:32:in `build' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations.rb:1531:in `has_many' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/book_destroy_async.rb:7:in `<class:BookDestroyAsync>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/book_destroy_async.rb:4:in `<module:Cpk>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/book_destroy_async.rb:3:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/activerecord/test/activejob/destroy_association_async_test.rb:27:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:61:in `block in load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `each' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:52:in `run' /Users/yahonda/src/github.com/rails/rails/activerecord/test/support/tools.rb:37:in `<top (required)>' bin/test:11:in `require_relative' bin/test:11:in `<main>' DEPRECATION WARNING: Setting `query_constraints:` option on `Cpk::ChapterDestroyAsync.belongs_to :book` is deprecated. To maintain current behavior, use the `foreign_key` option instead. (called from new at /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19) /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `new' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/reflection.rb:19:in `create' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:50:in `create_reflection' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations/builder/association.rb:32:in `build' /Users/yahonda/src/github.com/rails/rails/activerecord/lib/active_record/associations.rb:1910:in `belongs_to' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/chapter_destroy_async.rb:8:in `<class:ChapterDestroyAsync>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/chapter_destroy_async.rb:4:in `<module:Cpk>' /Users/yahonda/src/github.com/rails/rails/activerecord/test/models/cpk/chapter_destroy_async.rb:3:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/activerecord/test/activejob/destroy_association_async_test.rb:28:in `<top (required)>' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require' /Users/yahonda/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:61:in `block in load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `each' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:60:in `load_tests' /Users/yahonda/src/github.com/rails/rails/railties/lib/rails/test_unit/runner.rb:52:in `run' /Users/yahonda/src/github.com/rails/rails/activerecord/test/support/tools.rb:37:in `<top (required)>' bin/test:11:in `require_relative' bin/test:11:in `<main>' Run options: --seed 24461 .................... Finished in 0.199597s, 100.2019 runs/s, 370.7471 assertions/s. 20 runs, 74 assertions, 0 failures, 0 errors, 0 skips % ```

andyatkinson · 2024-05-31T16:14:52Z

Hey there @nvasilevski! I wanted to explore CPKs and the query_constraints option as currently defined in Rails 7.1. I made a single file Rails app and used the Author and Book models from the Rails Guide examples, with Postgres as my database.

Without a CPK definition and the query_constraints option, I was writing and reading records for both types. However, since adding this configuration verbatim from Rails Guides, I'm seeing invalid SQL generated when trying to create the descendent model (Book) through the author object "books" association.

Are you able to take a peek so we can rule out any misconfiguration on my part?

App file: https://github.com/andyatkinson/bookshop/blob/main/bookshop.rb

In the Readme I showed how to create the bookshop_development postgres DB using createdb. It just needs to exist before the app is booted. Here are instructions to bot the app, which goes straight into a rails console so we can start creating and loading models/records: https://github.com/andyatkinson/bookshop?tab=readme-ov-file#start-rails-console

I wrote up the issue I'm seeing with invalid SQL being generated here: https://github.com/andyatkinson/bookshop/blob/main/issue_invalid_sql.md

Let me know if this configuration is wrong. If the configuration looks ok, let me know if you'd like me to open a bug ticket in rails/rails.

zzak · 2024-05-31T23:52:39Z

@andyatkinson What happens if you load_defaults?

andyatkinson · 2024-06-04T14:45:10Z

Thanks for responding @zzak. I tried config.load_defaults 7.1 within the small app config block and from within the booted rails console, but unfortunately I got the same invalid SQL generated.

https://github.com/andyatkinson/bookshop/pull/1/files#diff-083d33321aa2caa0933877b1cd51c6186accf4e2073fb5b4a1818f87ee12837cR37

I guess this might be a quirk of the single file app, maybe something isn't being loaded that allows this to work, that's loaded in the full generated rails app. I might have to abandon the single file app for now since my goal was more to show the changes to the query_constraints / foreign keys option between 7.1 and 7.2. However, if anyone else has ideas, I'd love to try them out. I may also try and review the single file test reproduction app that the Rails team maintains.

nvasilevski · 2024-06-04T14:45:20Z

Hey @andyatkinson, it's almost certainly due to author_id column while setup expects [:author_first_name, :author_last_name] to be a composite foreign key so the books must have these columns.

It should work if you tweak the table definition to

  create_table :books, force: true do |t|
    t.text(:title, null: false)
    t.text(:author_first_name, null: false)
    t.text(:author_last_name, null: false)
  end

and association definition to

class Author < ActiveRecord::Base
  self.primary_key = [:first_name, :last_name]
  has_many :books, query_constraints: [:author_first_name, :author_last_name]
end

From Rails 7.2 query_constraints can be changed to foreign_key: [:author_first_name, :author_last_name] which hopefully makes it easier to argue about the setup as it makes it clear what foreign key we want to use.

If the intention was to use author_id as a single-column foreign key then query_constraints needs to be ommited from the association and primary_key needs to be set to only use :id for the purpose of association, like: has_many :books, primary_key: :id

Let me know if any of these suggestions work and I'd be happy to clarify anything further. Thanks!

andyatkinson · 2024-06-04T14:58:44Z

Ah I see @nvasilevski, thank you for that. I was able to get examples going for both single column PKs / FKs, and multi-column/composite primary keys/foreign keys as defined at the app level:

With each of these table definitions and code configurations, I was able to run the examples in the Rails Guide successfully, creating an author, and a book scoped to the author.

Single column: andyatkinson/bookshop@9818564

CPKs: andyatkinson/bookshop@f3688cd

I'm going to see if there's an opportunity to contribute to the docs a bit here, and I also want to add to the example using DB-defined FKs as well for completeness.

Thanks for the help @nvasilevski and @zzak!

rails-bot bot added activerecord docs labels Apr 15, 2024

eileencodes requested changes Apr 17, 2024

View reviewed changes

eileencodes added this to the 7.2.0 milestone Apr 17, 2024

nvasilevski force-pushed the deprecate-explicit-query-constraints-in-favor-of-foreign-key branch 3 times, most recently from c96b4c7 to ad13455 Compare April 17, 2024 14:40

nvasilevski requested a review from eileencodes May 7, 2024 19:32

eileencodes approved these changes May 8, 2024

View reviewed changes

nvasilevski force-pushed the deprecate-explicit-query-constraints-in-favor-of-foreign-key branch from ad13455 to 9cadf61 Compare May 8, 2024 20:08

eileencodes approved these changes May 8, 2024

View reviewed changes

eileencodes merged commit 07e40ea into rails:main May 8, 2024
3 of 4 checks passed

eileencodes deleted the deprecate-explicit-query-constraints-in-favor-of-foreign-key branch May 8, 2024 20:22

yahonda mentioned this pull request May 13, 2024

Use foreign_key option for destroy_async test models #51806

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn about changing `query_constraints:` behavior #51571

Warn about changing `query_constraints:` behavior #51571

nvasilevski commented Apr 15, 2024

eileencodes Apr 17, 2024

nvasilevski Apr 17, 2024

eileencodes Apr 17, 2024

nvasilevski Apr 17, 2024

eileencodes Apr 17, 2024

eileencodes Apr 17, 2024

nvasilevski Apr 17, 2024

eileencodes left a comment

nvasilevski commented May 8, 2024

andyatkinson commented May 31, 2024

zzak commented May 31, 2024

andyatkinson commented Jun 4, 2024

nvasilevski commented Jun 4, 2024

andyatkinson commented Jun 4, 2024

		@@ -1,3 +1,10 @@
		* Warn about changing `query_constraints:` behavior

	* Warn about changing `query_constraints:` behavior
	* In association option `query_constraints` is deprecated in favor of `foreign_key`.

Warn about changing query_constraints: behavior #51571

Warn about changing query_constraints: behavior #51571

Conversation

nvasilevski commented Apr 15, 2024

Deprecation location

Test models

eileencodes Apr 17, 2024

Choose a reason for hiding this comment

nvasilevski Apr 17, 2024

Choose a reason for hiding this comment

eileencodes Apr 17, 2024

Choose a reason for hiding this comment

nvasilevski Apr 17, 2024

Choose a reason for hiding this comment

eileencodes Apr 17, 2024

Choose a reason for hiding this comment

eileencodes Apr 17, 2024

Choose a reason for hiding this comment

nvasilevski Apr 17, 2024

Choose a reason for hiding this comment

eileencodes left a comment

Choose a reason for hiding this comment

nvasilevski commented May 8, 2024

andyatkinson commented May 31, 2024

zzak commented May 31, 2024

andyatkinson commented Jun 4, 2024

nvasilevski commented Jun 4, 2024

andyatkinson commented Jun 4, 2024

Warn about changing `query_constraints:` behavior #51571

Warn about changing `query_constraints:` behavior #51571